Skip to content

Conversation

@zhiweiyin318
Copy link
Member

@zhiweiyin318 zhiweiyin318 commented Jun 18, 2025

Summary

the HubRegistrationDegraded condition in clusterManager only indicates the registration controller deployment status, need to indicates the registration webhook deployment status.

Related issue(s)

Fixes #

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced status reporting to monitor multiple registration deployment components and accurately reflect health status across all instances.
  • Tests

    • Added test coverage for webhook deployment scenarios and multi-component status validation.

@codecov
Copy link

codecov bot commented Jun 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.24%. Comparing base (da18ab2) to head (a9b2580).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1043      +/-   ##
==========================================
- Coverage   61.28%   61.24%   -0.05%     
==========================================
  Files         209      209              
  Lines       20881    20886       +5     
==========================================
- Hits        12797    12791       -6     
- Misses       6967     6977      +10     
- Partials     1117     1118       +1     
Flag Coverage Δ
unit 61.24% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link

This pull request is stale because it has been open for 120 days with no activity. After 14 days of inactivity, it will be closed. Remove the stale label to prevent this issue from being closed.

@github-actions github-actions bot added the Stale label Oct 16, 2025
@qiujian16
Copy link
Member

@zhiweiyin318 I think you can rebase this PR?

@coderabbitai
Copy link

coderabbitai bot commented Oct 16, 2025

Walkthrough

The status controller's updateStatusOfRegistration function now evaluates multiple registration deployments (registration-controller and registration-webhook) instead of a single deployment, checking each for retrieval errors and unavailable pods to determine the registration status condition.

Changes

Cohort / File(s) Summary
Registration Status Controller Enhancement
pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller.go
Modified updateStatusOfRegistration to iterate through multiple registration deployment names, fetch each deployment, and return a degraded condition on retrieval errors or if any deployment has unavailable pods. Falls through to non-degraded condition if all deployments are healthy.
Registration Status Controller Tests
pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller_test.go
Added helper function newRegistrationWebhookDeployment to create webhook deployment fixtures. Introduced new test case validating degraded registration condition when webhook pods are unavailable while placement remains functional. Extended existing test scenario to include webhook deployment in the deployments list.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description follows the required template structure with Summary and Related issue(s) headings but leaves a placeholder “Fixes #” without providing an actual issue number, so it lacks the necessary linkage to a specific issue. Please update the Related issue(s) section to include a valid issue number after ‘Fixes #’ or remove the placeholder if there is no associated issue.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (1 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly states that the PR adds a check for the registration webhook deployment when evaluating the HubRegistrationDegraded condition, directly reflecting the main change introduced by the code. It is concise, specific, and appropriately uses the seedling icon as prescribed for miscellaneous updates.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da18ab2 and a9b2580.

📒 Files selected for processing (2)
  • pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller.go (1 hunks)
  • pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller_test.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller_test.go (4)
pkg/common/testing/assertion.go (1)
  • AssertActions (51-61)
vendor/open-cluster-management.io/api/operator/v1/types_klusterlet.go (1)
  • Klusterlet (19-28)
pkg/operator/helpers/testing/assertion.go (2)
  • NamedCondition (17-19)
  • AssertOnlyConditions (21-47)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (2)
  • ConditionHubRegistrationDegraded (548-548)
  • ConditionHubPlacementDegraded (550-550)
pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller.go (2)
vendor/open-cluster-management.io/api/operator/v1/types_clustermanager.go (3)
  • ConditionHubRegistrationDegraded (548-548)
  • ReasonGetRegistrationDeploymentFailed (580-580)
  • ReasonUnavailableRegistrationPod (582-582)
pkg/operator/helpers/helpers.go (1)
  • NumOfUnavailablePod (334-345)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
  • GitHub Check: integration
  • GitHub Check: verify
  • GitHub Check: e2e-singleton
  • GitHub Check: build
  • GitHub Check: unit
  • GitHub Check: e2e-hosted
  • GitHub Check: e2e
🔇 Additional comments (4)
pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller.go (1)

91-116: LGTM! Multi-deployment health check implemented correctly.

The logic now properly checks both registration-controller and registration-webhook deployments, returning degraded status if either deployment fails or has unavailable pods. The early-return pattern ensures that any single unhealthy deployment correctly marks the overall registration as degraded.

pkg/operator/operators/clustermanager/controllers/statuscontroller/clustermanager_status_controller_test.go (3)

62-75: LGTM! Helper function consistent with existing pattern.

The new helper function follows the same structure as newRegistrationDeployment and correctly creates the webhook deployment with the expected naming convention.


163-184: LGTM! New test case validates webhook deployment health check.

The test correctly verifies that when the registration webhook has unavailable pods while the registration controller and placement are healthy, the registration condition is properly marked as degraded.


189-189: LGTM! Test case correctly updated for multi-deployment scenario.

The test now properly includes both registration deployments as healthy to verify that registration is functional when all deployments are available.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@zhiweiyin318
Copy link
Member Author

@zhiweiyin318 I think you can rebase this PR?

@qiujian16 done

@qiujian16
Copy link
Member

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 16, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16, zhiweiyin318

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 4a8b2dd into open-cluster-management-io:main Oct 16, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants